feat(sagemaker): add progress indicator during space remote connection process #8247
Conversation
|
bhavya2109sharma
changed the title
|
| title: `Connecting to ${spaceName}`, | ||
| }, | ||
| async (progress) => { | ||
| progress.report({ message: 'Starting the space. This can take around 2 minutes.' }) |
There was a problem hiding this comment.
How do we get the number? if it's a larger type would it be much longer?
There was a problem hiding this comment.
I got it from UXD
|
Thanks for making this fix, appreciate it! |
| title: `Connecting to ${spaceName}`, | ||
| }, | ||
| async (progress) => { | ||
| progress.report({ message: 'Starting the space. This can take around 2 minutes.' }) |
There was a problem hiding this comment.
"This can take around 2 minutes" is a hard thing to justify or communicate due to the variability (10s to >5mins depending on image). For now, would just do
"Starting space {spaceName}..."
Good intent but when this done in the web UI also users were left confused and we had a lot of feedback. So until we figure out a better message, lets avoid it.
There was a problem hiding this comment.
Sure I will remove it for now
There was a problem hiding this comment.
So msg will appear with the change like this : Connecting to ${spaceName} : Starting the space
is this fine ?
|
/retryBuilds |
| // Ignore InstanceTypeError since it means the user decided not to use an instanceType with more memory | ||
| if (err.code !== InstanceTypeError) { | ||
|
|
||
| const spaceName = node.spaceApp.SpaceName! |
There was a problem hiding this comment.
it seems to be a nullable, are we sure all the functions below which use this field handle the null case
There was a problem hiding this comment.
what will happen if this is null/undefined
There was a problem hiding this comment.
spaceName cannot be null because spaces must have names when created
| // In case of SMUS, we pass in a SM Client and for SM AI, it creates a new SM Client. | ||
| const client = sageMakerClient ? sageMakerClient : new SagemakerClient(node.regionCode) | ||
|
|
||
| try { |
There was a problem hiding this comment.
sry what's the purpose of having 2 layers of try...catch?
cant' we use 1?
try {
} catch() {
if (error.code === InstanceTypeError) {
}
}
There was a problem hiding this comment.
removed it.
There was a problem hiding this comment.
It appears previously we were logging all uncaught errors. Do we no longer want to do that? I just want to make sure removing that was intentional.
getLogger().error(`sm:openRemoteConnect: ${err}`)
There was a problem hiding this comment.
It was not there before more change. First I thought of having uncaught errors throws as well.
Now with this change only Progress Indicator is in place
|
/retryBuilds |
7 participants
Problem
Users had no idea what was happening during connecting to remote space, which can take few seconds to connect.
User clicks "Connect" → Nothing visible happens for few seconds → Either success or failure
Solution
Adds progress tracking for SageMaker space connections which shows connection status with space name in progress dialog
Test cases are not being added because it is just showing the user about progress during Space operations for good user experience.
Testing
Tested for both SM-AI and SMUS Spaces.
Screen.Recording.2025-10-30.at.2.38.04.PM.mov
Screen.Recording.2025-10-30.at.2.37.20.PM.mov
feature/xbranches will not be squash-merged at release time.